Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize actors colors #773

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

Clarkszw
Copy link
Contributor

@Clarkszw Clarkszw commented Mar 26, 2023

I have added a new function check_color_range() `normalize_color()' in utils.py for #458
It does two things:

  1. check if the color array is in range [0, 1]
  2. if not, it will change it to red color [1, 0, 0] and print the out of bounds information normalize the color array.

I have written an unit test for this function and added in test_utils.py

All tests in test_actors.py and test_utils.py are passed :)

@Clarkszw Clarkszw changed the title Standardize actors colors, issue #458 Standardize actors colors Mar 26, 2023
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Clarkszw,

Thank you for starting this.

See below for some comments:

  1. Concerning docstring, I recommend you looking at the numpy style guide. Many python projects follow those rule which you are not following right now on this PR. here the link:
    https://numpydoc.readthedocs.io/en/latest/format.html

  2. Why red? I recommend you to normalize your color values to go from [0-255] to [0-1]

fury/tests/test_utils.py Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Clarkszw,

it looks much better. I need to play a bit more with this PR to make sure we do not fall in weird cases.

Thank you for this and I will give you an update next week.

@skoudoro
Copy link
Contributor

skoudoro commented Apr 6, 2023

also, can you solve the conflict in this PR ? thank you

@Clarkszw
Copy link
Contributor Author

Clarkszw commented Apr 7, 2023

I have no idea why it will happen..But the conflict has been fixed now:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants